Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OpenTracing Bridge - Initial implementation #211

Merged
merged 103 commits into from
Oct 24, 2019

Conversation

johananl
Copy link
Contributor

@johananl johananl commented Oct 9, 2019

General

This is an initial implementation for the OpenTracing bridge for the Python OpenTelemetry library. There are still some things I would like to handle before considering a merge. On the other hand this is a big PR and I would like to start getting feedback as soon as possible, so I'm opening a draft PR while I work on closing the remaining work items (see TODO section below).

This PR supersedes #120. I have found it easier to open a new PR rather than altering the old one due to a significantly different implementation. Please tell me if I should edit the original PR instead.

Implementation

The PR adds an OpenTracing "shim". The shim is a collection of wrapper classes which should allow an OpenTracing user to instantiate an OpenTelemetry tracer and use it as if it were an OpenTracing tracer, without altering instrumentation code.

TODO

The following should probably be done before merging:

  • Squash commits. The current commits aren't properly organized so we should rebase and/or squash them before merging. I'm not sure how granular the commits should be - one big commit for the entire feature? A couple of smaller commits which are divided logically?
  • Add documentation. I want to ensure all the new classes and functions have Sphinx-friendly docstrings. Additionally, we should generate the required Sphinx files to have proper docs for the bridge.
  • Add typing. Following my conversation with @c24t, it could be nice to have typing info at least for the bridge's public API. This may require writing a typing stub for OpenTracing since the library doesn't currently include typing information.

The following features haven't been implemented yet:

  • Inject/extract methods. I haven't got to implementing them yet but there is no blocker for doing so AFAICT.
  • Baggage items. The spec doesn't specify baggage support yet, which is probably a blocker.

Testing

To run the unit tests, execute tox. Following are full instructions for running the tests from scratch, in case somebody finds it useful:

git clone git@github.com:kinvolk/opentelemetry-python.git
cd opentelemetry-python
git checkout johananl/ot-shim
virtualenv /tmp/venv
source /tmp/venv/bin/activate
pip install tox
tox

To use the bridge for tracing a dummy app, use the following:

git clone git@github.com:kinvolk/opentelemetry-python.git
cd opentelemetry-python
git checkout johananl/ot-shim
virtualenv /tmp/venv
source /tmp/venv/bin/activate
pip install ext/opentelemetry-ext-opentracing-shim
# An OpenTelemetry tracer is necessary. I've used the SDK here but a different tracer should also work.
pip install opentelemetry-sdk

Then, instrument an app:

import time
from opentelemetry.sdk.trace import Tracer, export
from opentelemetry.sdk.trace.export.in_memory_span_exporter import InMemorySpanExporter
from opentelemetry.ext.opentracing_shim import create_tracer

otel_tracer = Tracer()
memory_exporter = InMemorySpanExporter()
span_processor = export.SimpleExportSpanProcessor(memory_exporter)
otel_tracer.add_span_processor(span_processor)

tracer = create_tracer(otel_tracer)

print('------------Started------------')

with tracer.start_active_span('parent') as parent:
    time.sleep(0.1)
    with tracer.start_active_span('child') as child:
        time.sleep(0.3)

for s in memory_exporter.get_finished_spans():
    print(s)
    print(s.get_context())

print('-------------Done--------------')

@johananl
Copy link
Contributor Author

johananl commented Oct 9, 2019

Looks like reviewers don't get tagged in draft PRs. Changing to a regular PR.

@johananl johananl marked this pull request as ready for review October 9, 2019 18:41
@johananl johananl mentioned this pull request Oct 9, 2019
6 tasks
@reyang
Copy link
Member

reyang commented Oct 9, 2019

Do we want to make this a top level folder? I suggest that we move it to the ext folder.

@c24t
Copy link
Member

c24t commented Oct 9, 2019

Nice work! Thanks @johananl.

The current commits aren't properly organized so we should rebase and/or squash them before merging.

If you merge via github we have this repo configured to automatically squash the branch into a single commit, you just have to remember to fix the ugly generated commit message.

I think it's fine to split inject/extract and baggage into separate PRs.

@johananl johananl force-pushed the johananl/ot-shim branch 2 times, most recently from 3c3766a to af1ff0e Compare October 10, 2019 10:32
Copy link
Member

@Oberon00 Oberon00 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition:

opentracing-shim/setup.py Outdated Show resolved Hide resolved
opentracing-shim/setup.py Outdated Show resolved Hide resolved
opentracing-shim/setup.py Outdated Show resolved Hide resolved
opentracing-shim/src/opentracingshim/__init__.py Outdated Show resolved Hide resolved
opentracing-shim/src/opentracingshim/__init__.py Outdated Show resolved Hide resolved
opentracing-shim/src/opentracingshim/__init__.py Outdated Show resolved Hide resolved
opentracing-shim/src/opentracingshim/__init__.py Outdated Show resolved Hide resolved
opentracing-shim/src/opentracingshim/util.py Outdated Show resolved Hide resolved
opentracing-shim/tests/test_shim.py Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
Copy link
Member

@c24t c24t left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. After reviewing I see how type annotations could be helpful here -- the code gets confusing quickly when we've got two different packages with the same class names.

Descriptions for args and return values in docstrings for non-API methods would be helpful. So would a module docstring or README that describes how to use the shim.

It'll be especially helpful to see tests or examples that show code instrumented with only the OpenTracing API, but create OpenTelemetry objects under the hood.

None of my comments are blocking, but I still see a few from @Oberon00. I'll review again and stamp it once those are resolved.

@reyang
Copy link
Member

reyang commented Oct 11, 2019

Maybe a dumb question - what's the difference between bridge and shim here?

This PR seems to be focusing on the OpenTracing shim, which means one can use the shim to convert an OpenTelemetry tracer into OpenTracing tracer.

A bridge sounds like bi-directional thing, which also means one can turn an OpenTracing tracer into an OpenTelemetry one.

@johananl
Copy link
Contributor Author

Maybe a dumb question - what's the difference between bridge and shim here?

@reyang that's a great question actually. I've been using the two terms interchangeably. In the code I've stuck to "shim" for consistency, however in issues etc. I've seen people use both.

I don't have a strong opinion on which one to use, as long as we're consistent with it. We should also remember we have a similar term in other languages.

- Remove an unnecessary context manager layers.
- Allow creating `ContextWrapper` objects either from a `SpanWrapper`
  or from a `Span` context manager.
@johananl
Copy link
Contributor Author

johananl commented Oct 23, 2019

Rebased over current master. @carlosalberto, could you please merge if things look OK to you? Editing the squashed commit message is going to be a mess... I guess we can call it "Add initial OpenTracing bridge implementation" or something similar.

Until we figure out how to handle unsupported frameworks (e.g.
gevent), we remove the `scope_manager` argument of `create_tracer`
to avoid breaking the OpenTelemetry context propagation.
These are obvious and therefore unnecessary.
The API docs are clear about the `references` argument being a list.
We don't try to compensate for wrong argument types elsewhere, so no
reason to do that here.
Not all classes in the shim are actually wrappers, which makes the
XWrapper naming convention confusing. XShim is more neutral.
In the OpenTracing API, when an exception occurs while a span is
active, information about the exception is added to the span as logs
and a tag is added as well. We re-add this functionality to the shim
since we've overridden the `__exit__()` method in which it lives.
Validating the parent type complicates the logic of `start_span()`.
Since the OpenTracing API clearly specifies the valid types for a
parent span, it's the user's responsibility to specify a valid
parent.
Verify that calling `close()` on a `ScopeShim` deactivates the span
in the OpenTelemetry tracer.
We shouldn't return OpenTelemetry objects to the OpenTracing API.
Make it clearer that the provided tracer object should be an
OpenTelemetry tracer, not an OpenTracing tracer.
@c24t
Copy link
Member

c24t commented Oct 23, 2019

Editing the squashed commit message is going to be a mess...

@carlosalberto please delete the generated commit message or rewrite it as a nice summary when you merge! No more ugly commits on master. :D

@carlosalberto carlosalberto merged commit e4d8949 into open-telemetry:master Oct 24, 2019
@carlosalberto
Copy link
Contributor

@c24t done ;)

mauriciovasquezbernal added a commit to kinvolk/opentelemetry-python that referenced this pull request Nov 2, 2019
The example of the jaeger exporter is failing because
e4d8949 ("OpenTracing Bridge - Initial implementation (open-telemetry#211)") introduced
a new timestamp parameter to add_event, the example was passing parameter by
position so it was messed up.
mauriciovasquezbernal added a commit to kinvolk/opentelemetry-python that referenced this pull request Nov 5, 2019
e4d8949 ("OpenTracing Bridge - Initial implementation (open-telemetry#211)") introduced
a new timestamp argument to the add_event method. This commit moves that
argument to be the last one because it is more common to have event attributes
than an explicitly timestamp.
c24t pushed a commit that referenced this pull request Nov 5, 2019
e4d8949 ("OpenTracing Bridge - Initial implementation (#211)") introduced
a new timestamp argument to the add_event method. This commit moves that
argument to be the last one because it is more common to have event attributes
than an explicitly timestamp.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants